Allow charts to be optional when deploying trento-web #17

rtorrero · 2024-01-16T14:59:03Z

This PR makes use of the env var added in trento-project/web#2147 to disable the usage of the new charts feature by introducing the enable_charts var. The variable has a default value of true (it shouldn't be necessary to set it true as it's already true by default in trento-web but I think it helps to make it more obvious)

@CDimonaco something I noticed is that you used CHARTS_ENABLED for the env var, where we usually use the form ENABLE_<something> for such things, if it's ok by you, could we change this for uniformity?

CDimonaco · 2024-01-17T11:51:19Z

@rtorrero the enable of charts should be an and condition between the availability of prometheus and the charts enabling, otherwise we could end in inconsistencies

rtorrero · 2024-01-17T12:00:09Z

@rtorrero the enable of charts should be an and condition between the availability of prometheus and the charts enabling, otherwise we could end in inconsistencies

I thought about this, but what if a user wants to use his existing prometheus installation instead of having us deploy ours? The other way around is also applicable (though a bit more unlikely): what if a user wants to collect metrics with prometheus but doesn't want the charts feature?

CDimonaco · 2024-01-17T13:39:38Z

I think this is an edge case scenario, it's true that the user can provide his prometheus, so we can say that it's true when the charts enabled variable is true AND the install prometheus variable or whatever is provided, so false or true we have the user that has an explicit choice, wdyt?

rtorrero · 2024-01-17T15:56:20Z

I think this is an edge case scenario, it's true that the user can provide his prometheus, so we can say that it's true when the charts enabled variable is true AND the install prometheus variable or whatever is provided, so false or true we have the user that has an explicit choice, wdyt?

I think I get what you mean, is the following correct?

| CHARTS_ENABLED                     | PROMETHEUS_ENABLED   | Outcome                                                                             |
|---------------------------------------------|--------------------------------------|-----------------------------------------------------------------------------|
| true                                               | true                                       | Charts are enabled, Prometheus is provisioned          |
| true                                               | false                                      | Charts are enabled, Prometheus is not  provisioned  |
| false                                              | true                                       | Charts are disabled, Prometheus is provisioned         |
| false                                              | false                                      | Both Charts and Prometheus are disabled                   |
| <unspecified>                             | true                                       | Charts are enabled,  Prometheus is provisioned         |
| <unspecified>                             | false                                      | Charts are disabled, Prometheus is not provisioned  |                    
| true                                               | <unspecified>                     | Charts are enabled, Prometheus is provisioned          |
| false                                              | <unspecified>                     | Charts are disabled, Prometheus is not provisioned  |

CDimonaco · 2024-01-18T09:55:53Z

I think this is an edge case scenario, it's true that the user can provide his prometheus, so we can say that it's true when the charts enabled variable is true AND the install prometheus variable or whatever is provided, so false or true we have the user that has an explicit choice, wdyt?

I think I get what you mean, is the following correct?

| CHARTS_ENABLED                     | PROMETHEUS_ENABLED   | Outcome                                                                             |
|---------------------------------------------|--------------------------------------|-----------------------------------------------------------------------------|
| true                                               | true                                       | Charts are enabled, Prometheus is provisioned          |
| true                                               | false                                      | Charts are enabled, Prometheus is not  provisioned  |
| false                                              | true                                       | Charts are disabled, Prometheus is provisioned         |
| false                                              | false                                      | Both Charts and Prometheus are disabled                   |
| <unspecified>                             | true                                       | Charts are enabled,  Prometheus is provisioned         |
| <unspecified>                             | false                                      | Charts are disabled, Prometheus is not provisioned  |                    
| true                                               | <unspecified>                     | Charts are enabled, Prometheus is provisioned          |
| false                                              | <unspecified>                     | Charts are disabled, Prometheus is not provisioned  |

Yep, let's keep in mind that in 99% of cases both of them will be true, and now in charts web code we have something to thing about multiple prometheus instance but it's not something related to the installation method

EMaksy

Hey man nice, thanks a lot. Just left a comment about the naming. Besides this --> LGTM!

arbulu89

I personally think that we are overdoing things.
If we put the descriptions of the variables correctly, they will be used OK.
So the enable_charts could be: Enable/Disable charts displayed based on Prometheus metrics. A prometheus installation is required, setting "provision_prometheus" to true or providing an externally installed prometheus url in "prometheus_url"

arbulu89 · 2024-01-22T15:32:54Z

playbook.yml

@@ -53,12 +53,10 @@

 - name: Provision prometheus
  become: true
-  vars:


why do you remove this?
what's the default value of it if the user doesn't set it?

If I keep the vars: section here it breaks the logic that checks if the value is defined to set a default if it's not. We can put it back as it was but this would break the behavior that Carmine suggested

arbulu89 · 2024-01-22T15:34:59Z

README.md

@@ -245,7 +245,7 @@ These variables are the defaults of our roles, if you want to override the prope
 | provision_postgres | Run the postgres provisioning contained into postgres role, set to false if you provide an external postgres to the services | "true" |
 | provision_rabbitmq | Run the rabbitmq provisioning contained into rabbitmq role, set to false if you provide an external rabbitmq to the services | "true" |
 | provision_proxy | Run the nginx provisioning for exposing all the services, se to false if you don't want to expose the services or you have already in place a reverse proxy infrastructure | "true" |
-| provision_prometheus | Run the prometheus provisioning used by trento to store metrics send by agents | "true" |
+| provision_prometheus | Run the prometheus provisioning used by trento to store metrics send by agents | true when enable_charts is true or undefined |


I think the default value should be true, The other conditions should be explained in the description.
The same goes for enable_charts.
The current descriptions are really difficult to understand for me at least

rtorrero · 2024-01-22T16:48:23Z

I personally think that we are overdoing things. If we put the descriptions of the variables correctly, they will be used OK. So the enable_charts could be: Enable/Disable charts displayed based on Prometheus metrics. A prometheus installation is required, setting "provision_prometheus" to true or providing an externally installed prometheus url in "prometheus_url"

I guess you are proposing here to decouple them and simply set them both to true by default? this was the original solution, for me it's up to @CDimonaco , I don't have a strong opinion here.

arbulu89 · 2024-01-23T08:04:18Z

@rtorrero If that was the original solution, yes, that's what I would do.
If somebody disables prometheus manually, without providing a installed prometheus url, leaving the charts enabled, I would say it is his fault.
Because anyone, even with this code, can put prometheus to false and charts enabled to true, and still have inconsistencies.
I think that the description is accurate, is all we need to do.

PD: as well, because this crossed dependency I see is extremely strange

CDimonaco · 2024-01-23T10:51:09Z

@arbulu89 I agree with you, we can go this way to simplify also the code

I was promoting the other way around but reflecting rn, it's better to go through the easy way

arbulu89

Thank you!

rtorrero added 2 commits January 16, 2024 14:42

Add enable_charts variable

d6f0160

Match env var name used in trento-web

2fe03f9

rtorrero requested review from CDimonaco, arbulu89 and EMaksy January 16, 2024 14:59

rtorrero marked this pull request as ready for review January 16, 2024 15:37

EMaksy approved these changes Jan 19, 2024

View reviewed changes

arbulu89 reviewed Jan 22, 2024

View reviewed changes

rtorrero force-pushed the allow-charts-optional branch from 18403fb to 2fe03f9 Compare January 23, 2024 18:43

rtorrero requested a review from arbulu89 January 23, 2024 18:45

arbulu89 approved these changes Jan 24, 2024

View reviewed changes

CDimonaco approved these changes Jan 24, 2024

View reviewed changes

rtorrero merged commit b91ad0a into main Jan 24, 2024
10 checks passed

stefanotorresi deleted the allow-charts-optional branch January 25, 2024 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow charts to be optional when deploying trento-web #17

Allow charts to be optional when deploying trento-web #17

rtorrero commented Jan 16, 2024

CDimonaco commented Jan 17, 2024

rtorrero commented Jan 17, 2024

CDimonaco commented Jan 17, 2024

rtorrero commented Jan 17, 2024 •

edited

Loading

CDimonaco commented Jan 18, 2024

EMaksy left a comment

arbulu89 left a comment

arbulu89 Jan 22, 2024

rtorrero Jan 22, 2024

arbulu89 Jan 22, 2024 •

edited

Loading

rtorrero commented Jan 22, 2024

arbulu89 commented Jan 23, 2024 •

edited

Loading

CDimonaco commented Jan 23, 2024

arbulu89 left a comment

Allow charts to be optional when deploying trento-web #17

Allow charts to be optional when deploying trento-web #17

Conversation

rtorrero commented Jan 16, 2024

CDimonaco commented Jan 17, 2024

rtorrero commented Jan 17, 2024

CDimonaco commented Jan 17, 2024

rtorrero commented Jan 17, 2024 • edited Loading

CDimonaco commented Jan 18, 2024

EMaksy left a comment

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

arbulu89 Jan 22, 2024

Choose a reason for hiding this comment

rtorrero Jan 22, 2024

Choose a reason for hiding this comment

arbulu89 Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

rtorrero commented Jan 22, 2024

arbulu89 commented Jan 23, 2024 • edited Loading

CDimonaco commented Jan 23, 2024

arbulu89 left a comment

Choose a reason for hiding this comment

rtorrero commented Jan 17, 2024 •

edited

Loading

arbulu89 Jan 22, 2024 •

edited

Loading

arbulu89 commented Jan 23, 2024 •

edited

Loading